-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deploy: Try to rebuild policy in new deployment if needed #2569
deploy: Try to rebuild policy in new deployment if needed #2569
Conversation
Hi @WOnder93. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
BTW, I did test this in a dummy Fedora CoreOS VM where I reproduced the issue (basically by installing a dummy SELinux module and then doing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot for working on this!
It looks like this is updating the libglnx submodule unintentionally.
if (!g_spawn_check_exit_status(exit_status, NULL)) | ||
g_message ("Failed to refresh SELinux policy - the policy contents may be inconsistent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you made this non-fatal to handle ratcheting it in? Let's figure out instead how to auto-detect it so we can tighten this. Two obvious options:
- Feature detection at runtime: e.g. search for switch in output of
semodule --help
? This would be best I think but can get quite verbose. - Compile-time version check: add autoconf goop to detect if
libselinux
is high enough? Hmm, it looks like on Fedorasemodule
is part ofpolicycoreutils
. But they're all versioned together, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second option won't work, because we depend on a given semodule
version inside the deployment, which may not match the runtime where the ostree
is running (may be an older/newer/completely different deployment).
The first option I had thought off, it just didn't seem worth it. But since you request it, I'll try to do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd vote to key this off an environment variable; we already have OSTREE_SYSROOT_OPTS
, so it could be a new OSTREE_SYSROOT_OPTS=semodule-hard-fail
to start. In the FCOS/RHCOS side we can manage rolling that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already went with scanning the --help output, which turned out to be not that verbose nor ugly. Please have a look and let me know if it works for you.
It's not entirely clear to me how it would work with the env variable, but the idea doesn't sound very appealing to me... Yet, it's not up to me so I'll submit to whatever you guys prefer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this!
"--bind", "var", "/var", | ||
"--symlink", "/usr/lib", "/lib", | ||
"--symlink", "/usr/lib32", "/lib32", | ||
"--symlink", "/usr/lib64", "/lib64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One messy thing is this is distribution-specific. Looks like there's some bits related to this here too https://github.com/flatpak/flatpak/blob/aac1205d66e03facc965279d5825272597b305d0/common/flatpak-run.c#L77
I think it's OK for now, but we will likely need to be updating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming in to this late, but why not do an --ro-bind
of .
to /
followed by the writable binds for /usr
, /etc
and /var
? Presumably the usr-merge symlinks are already setup properly in the deployment.
if (!g_spawn_check_exit_status(exit_status, NULL)) | ||
g_message ("Failed to refresh SELinux policy - the policy contents may be inconsistent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd vote to key this off an environment variable; we already have OSTREE_SYSROOT_OPTS
, so it could be a new OSTREE_SYSROOT_OPTS=semodule-hard-fail
to start. In the FCOS/RHCOS side we can manage rolling that out.
Tangentially related but I want to mention; on the coreos side we've been pulling things in a whole new direction with https://github.com/coreos/enhancements/blob/main/os/coreos-layering.md (There the policy rebuild would happen in a container and be applied centrally, not happen per host/machine) EDIT: To be clear I think we do want both; it should work to build a custom image but e.g. test out a per-machine |
a7e8064
to
51bbb72
Compare
Hmm, the transactionality test failed...but passed on rerun. Unfortunately it seems we don't save logs from the failing case, grr. Offhand...it seems like this should be transactional. But let me see if I can replicate any failures locally. |
Whenever the user has SELinux enabled and has any local modules/modifications installed, it is necessary to rebuild the policy in the final deployment, otherwise ostree will leave the binary policy files unchanged from last deployment as it detects difference against the base content (in rpm-ostree case this is the RPM content). To avoid the situation where the policy binaries go stale once any local customization of the policy is made, try to rebuild the policy as part of sysroot_finalize_deployment(). Use the special --rebuild-if-modules-changed switch, which detects if the input module files have changed relative to last time the policy was built and skips the most time-consuming part of the rebuild process if modules are unchanged (thus making this a relatively cheap operation if the user hasn't made any modifications to the shipped policy). As suggested by Jonathan Lebon, this uses bubblewrap (via g_spawn_sync()) to perform the rebuild inside the deployment's filesystem tree, which also means that ostree will have a runtime dependency on bubblewrap. Partially addresses: coreos/fedora-coreos-tracker#701 Signed-off-by: Ondrej Mosnacek <[email protected]>
51bbb72
to
edb4f38
Compare
Let's log when we don't find the expected CLI argument which will help debug things.
Our merge process doesn't squash Added a commit on top for more logging. Now that I look, we don't have the updated In the meantime though I think we can merge this. |
Then maybe CONTRIBUTING.md needs updating? ;)
🎉 |
Just to xref, openshift/os@e974146 landed which is a worse version of this - in particular the policy is only rebuilt early during the boot, at which point we may already be subject to problems with drift. This PR rebuilds before booting, which avoids those issues. |
Whenever the user has SELinux enabled and has any local
modules/modifications installed, it is necessary to rebuild the policy
in the final deployment, otherwise ostree will leave the binary policy
files unchanged from last deployment as it detects difference against
the base content (in rpm-ostree case this is the RPM content).
To avoid the situation where the policy binaries go stale once any local
customization of the policy is made, try to rebuild the policy as part
of sysroot_finalize_deployment(). Use the special
--rebuild-if-modules-changed switch, which detects if the input module
files have changed relative to last time the policy was built and skips
the most time-consuming part of the rebuild process if modules are
unchanged (thus making this a relatively cheap operation if the user
hasn't made any modifications to the shipped policy).
As suggested by Jonathan Lebon, this uses bubblewrap (via
g_spawn_sync()) to perform the rebuild inside the deployment's
filesystem tree, which also means that ostree will have a runtime
dependency on bubblewrap.
Partially addresses: coreos/fedora-coreos-tracker#701